fix(tokens): normalize inverted bbox in get_location instead of asserting - #676
Open
maxtaran2010 wants to merge 2 commits into
Open
fix(tokens): normalize inverted bbox in get_location instead of asserting#676maxtaran2010 wants to merge 2 commits into
maxtaran2010 wants to merge 2 commits into
Conversation
…ting Near-degenerate elements (e.g. a thin rule whose layout-model regression produced a slightly inverted bbox) could arrive with bbox[0] > bbox[2] or bbox[1] > bbox[3]. The redundant asserts in DocumentToken.get_location crashed export_to_doctags(add_location=True) on such boxes, even though the method already normalizes ordering via min()/max() downstream. This bites long documents in particular, where the odds of one degenerate element grow with page count. Sort the coordinates before normalization to mirror the existing min()/max() behaviour, and drop the asserts. Adds regression tests for normal, partially inverted (from the issue repro), and fully inverted bboxes. Fixes docling-project#620 Signed-off-by: max <87073104+maxtaran2010@users.noreply.github.com>
Contributor
|
✅ DCO Check Passed Thanks @maxtaran2010, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🔴 1 of 2 protections blocking · waiting on 👀 reviews
🔴 Require two reviewer for test updatesWaiting for
This rule is failing.When test data is updated, we require two reviewers
Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates DocumentToken.get_location() to handle slightly inverted bounding boxes by normalizing coordinate order instead of asserting, preventing crashes during DocTags export for near-degenerate layout elements (Issue #620).
Changes:
- Normalize potentially inverted
bboxcoordinates usingsorted()and remove the crash-causing assertions. - Add unit tests covering normal, partially inverted (issue repro), and fully inverted bounding boxes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docling_core/types/doc/tokens.py | Normalizes inverted bbox coordinates before computing location tokens, avoiding asserts/crashes. |
| test/test_tokens.py | Adds regression/unit tests to ensure inverted bboxes serialize consistently and don’t crash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -303,13 +303,19 @@ def get_location( | |||
| self_closing: bool = False, | |||
| ): | |||
| """Get the location string give bbox and page-dim.""" | |||
Addresses review feedback on docling-project#676. Signed-off-by: max <87073104+maxtaran2010@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #620
Problem
`DocumentToken.get_location()` in `docling_core/types/doc/tokens.py` asserted `bbox[0] <= bbox[2]` and `bbox[1] <= bbox[3]`. Near-degenerate elements (e.g. a thin decorative rule whose layout-model regression produces a slightly inverted bbox) can violate this by a fraction of a point, which crashes `export_to_doctags(add_location=True)`:
The asserts were redundant: the method already emits correctly-ordered location tokens via `min()`/`max()` downstream. The probability of hitting one such element grows with page count, so long documents are the most affected.
Fix
Sort the coordinates before normalization (mirroring the existing `min()`/`max()` behaviour) and drop the asserts. Output is unchanged for well-formed boxes and now correct instead of crashing for inverted ones.
Tests
Added `test/test_tokens.py` covering:
All pass; existing `test_serialization_doctag.py` and `test_doctags_load.py` (17 tests) still pass, and `ruff` is clean.